- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 8.6k
[java] Fixed issue with embedded authentication in URLs for JdkHttpClient #15071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[java] Fixed issue with embedded authentication in URLs for JdkHttpClient #15071
Conversation
| PR Reviewer Guide 🔍Here are some key observations to aid the review process: 
 | 
| PR Code Suggestions ✨Explore these optional code suggestions: 
 | 
Signed-off-by: Viet Nguyen Duc <[email protected]>
Signed-off-by: Viet Nguyen Duc <[email protected]>
        
          
                java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | @VietND96 Please do not merge this, in my mind this will not fix the issue. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iampopovich have you tested this locally?
| @diemol sure | 
| Just wondering why @joerg1985 thinks this does not fix the issue. | 
| @diemol i will write later this evening details about this and the original issue. | 
| This PR does remove the UserInfo from the URI, this is okay, but it will not fix the issue #13803 from my point of view. I have asked inside the ticket #13803 for details about the setup, but never did get a response. In #13802 you can see a nginx response with 404, so my current assumption is the authentication is done by a reverse proxy not following the RFC for WWW-Authenticate. The Java  We would need to translate the UserInfo from the URI to a 'Authorization' header and send it blind to the server. This might end in other issues, e.g. we do not know the expected scheme and might send the plain credentials (Basic scheme) instead of a hashed one (Digest scheme) over a unencrypted connection. The nginx config might be an attemp to hide the service from attackers by forcing the clients to send the 'Authorization' header without a challenge. Some of the selenium language bindings might do so by default and IF it should be supported, this could be implemented in Java too. But this should be done via opt-in and not by default to avoid unexpected issues. And we should know the usecase so we can document this, to avoid more magic lines in the code nobody is aware of the use. I would propalby hide the service using a UUID prefix in the path, the server has a command line flag to add a prefix... | 
| @iampopovich, without this fix, for example a simple get started  | 
| 
 Actually, this kind of embedded auth URL works in Python binding. In background, it also extract  
 Probably, it is correct with @joerg1985 on the approach that should do. In JdkHttpClient, how to insert the Authorizationto request headers, I think you have to check it. | 
| 
 Looks like it works without this change, since I added a test to example site SeleniumHQ/seleniumhq.github.io#2131 | 
| @VietND96 What should we do next? Clearly, this PR does not fix the bug. I tried implementing authorization similar to the Python bindings client, but it didn’t solve the issue. At the moment, I don’t have enough knowledge to propose an alternative solution. We can close this pull request for now, and perhaps I’ll revisit the problem later if it’s still relevant. | 
User description
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
This pull request includes several changes to the
JdkHttpClientclass to handle and test the stripping of credentials from URLs. The most significant changes include modifications to theJdkHttpClientclass, the addition of new methods and fields, and the introduction of new tests to ensure the correct functionality.Updates to
JdkHttpClientclass:ClientConfigfield and initialized it in the constructor to store configuration settings.getPasswordAuthenticationmethod.getBaseUrifor testing purposes to retrieve the base URI from the configuration.sendmethod to include the cause in theWebDriverExceptionwhen interrupted.New tests in
JdkHttpClientTestclass:Motivation and Context
Fixes #13803
Types of changes
Checklist
PR Type
Bug fix, Tests
Description
Fixed issue with embedded authentication in URLs for
JdkHttpClient.Added logic to strip credentials from base URI in
JdkHttpClient.Introduced new tests to validate credential stripping and URL component preservation.
Enhanced exception handling in
sendmethod to include cause details.Changes walkthrough 📝
JdkHttpClient.java
Implement credential stripping and enhance exception handlingjava/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java
getBaseUrifor testing.sendmethod.ClientConfiginitialization to handle credential-free URIs.JdkHttpClientTest.java
Add tests for URL credential stripping and validationjava/test/org/openqa/selenium/remote/http/jdk/JdkHttpClientTest.java